Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Build Set and Map more efficiently and consistently #1042

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Build Set and Map more efficiently and consistently #1042

wants to merge 3 commits into from

Conversation

meooow25
Copy link
Contributor

@meooow25 meooow25 commented Sep 16, 2024 •
edited
Loading

Use "Builder"s to implement some Set and Map construction functions.
As a result, some have become good consumers in terms of list fusion,
and all are now O(n) for non-decreasing input.

                     Fusible  Fusible  O(n) for     O(n) for
                     before   after    before       after
Set.fromList         No       Yes      Strict incr  Non-decr
Set.map              -        -        Strict incr  Non-decr
Map.fromList         No       Yes      Strict incr  Non-decr
Map.fromListWith     Yes      Yes      Never        Non-decr
Map.fromListWithKey  Yes      Yes      Never        Non-decr
Map.mapKeys          -        -        Strict incr  Non-decr
Map.mapKeysWith      -        -        Never        Non-decr

For #1027. Also related to #949.

Benchmarks with GHC 9.10.1:

Set:

Name                    Time - - - - - - - -    Allocated - - - - -
                             A       B     %         A       B     %
fromList                        795 μs  784 μs   -1%    1.9 MB  1.9 MB   +0%
fromList-distinctAsc             50 μs   26 μs  -48%    271 KB  256 KB   -5%
fromList-distinctAsc:fusion      70 μs   29 μs  -58%    559 KB  415 KB  -25%
fromList-distinctDesc           559 μs  557 μs   +0%    2.6 MB  2.6 MB   +0%
fromList-distinctDesc:fusion    592 μs  604 μs   +2%    2.9 MB  2.9 MB   +0%
map:asc                          95 μs   75 μs  -20%    557 KB  414 KB  -25%
map:desc                        620 μs  620 μs   +0%    2.8 MB  2.7 MB   -4%

Map:

Name                           Time - - - - - - - -    Allocated - - - - -
                                    A       B     %         A       B     %
fromList                        838 μs  829 μs   -1%    2.3 MB  2.3 MB   +0%
fromList-distinctAsc             56 μs   30 μs  -46%    319 KB  303 KB   -4%
fromList-distinctAsc:fusion      81 μs   31 μs  -61%    702 KB  480 KB  -31%
fromList-distinctDesc           579 μs  582 μs   +0%    3.1 MB  3.1 MB   +0%
fromList-distinctDesc:fusion    609 μs  624 μs   +2%    3.5 MB  3.4 MB   -1%
fromListWith-asc                509 μs   35 μs  -93%    2.7 MB  272 KB  -90%
fromListWith-asc:fusion         543 μs   29 μs  -94%    3.1 MB  511 KB  -83%
fromListWith-desc               502 μs  528 μs   +5%    2.7 MB  2.8 MB   +3%
fromListWith-desc:fusion        539 μs  584 μs   +8%    3.1 MB  3.1 MB   +0%
fromListWithKey-asc             501 μs   31 μs  -93%    2.7 MB  288 KB  -89%
fromListWithKey-asc:fusion      542 μs   30 μs  -94%    3.1 MB  527 KB  -83%
fromListWithKey-desc            505 μs  540 μs   +6%    2.7 MB  2.8 MB   +4%
fromListWithKey-desc:fusion     532 μs  595 μs  +11%    3.2 MB  3.2 MB   +0%
mapKeys:asc                     137 μs   81 μs  -41%    864 KB  477 KB  -44%
mapKeys:desc                    706 μs  669 μs   -5%    3.6 MB  3.2 MB  -11%
mapKeysWith:asc                 750 μs   74 μs  -90%    3.2 MB  463 KB  -85%
mapKeysWith:desc                722 μs  599 μs  -16%    3.2 MB  2.9 MB  -10%

Note: You may notice desc fusion cases haven't improved, though they really should. It turns out to be a enumFromThen/fusion issue, and not really a problem with our code. I opened a GHC issue for it: GHC#25259

@meooow25
Copy link
Contributor Author

Hmm.. the failing strictness test is a bit silly. It is defeated by the new fusible definition. Anyway, it gets removed in #1021.

@treeowl
Copy link
Contributor

treeowl commented Sep 16, 2024

How do things go when fusion doesn't happen?

@treeowl
Copy link
Contributor

treeowl commented Sep 16, 2024

Is there any way to get side by side comparisons of times and allocations? All the befores followed by all the afters is hard to look at.

@meooow25
Copy link
Contributor Author

meooow25 commented Sep 17, 2024 •
edited
Loading

How do things go when fusion doesn't happen?

Seems to be around the same, perhaps slightly slower. You can see these in the table when there is no fusion. For example, Map's fromList-desc is -1% and fromListWith-desc is +4%.

Is there any way to get side by side comparisons of times and allocations? All the befores followed by all the afters is hard to look at.

Right, I had a script for that. Updated the benchmarks section.

@meooow25 meooow25 mentioned this pull request Sep 17, 2024
Merged
@meooow25 meooow25 mentioned this pull request Oct 4, 2024
Open
@meooow25
Copy link
Contributor Author

meooow25 commented Oct 4, 2024 •
edited
Loading

Another benefit of this implementation (which I did not consider before, #351) is that it will allow defining functions like fromFoldable since we are just doing a foldl' now. This will be more efficient in case there is a real list in the way, or if there isn't but list-fusion does not work out optimally.

Going further, we could expose the Builder stuff if there is a demand for it.

@meooow25 meooow25 mentioned this pull request Oct 31, 2024
Open
9 tasks
@meooow25 meooow25 changed the title Build Set and Map more efficiently Build Set and Map more efficiently and consistently Nov 1, 2024
@meooow25
Benchmarks for affected functions
9c34532
@meooow25
Property tests for affected functions
c2b3c15
@meooow25
Build Set and Map more efficiently
4c05817 
Use "Builder"s to implement some Set and Map construction functions.
As a result, some have become good consumers in terms of list fusion,
and all are now O(n) for non-decreasing input.

                     Fusible  Fusible  O(n) for     O(n) for
                     before   after    before       after
Set.fromList         No       Yes      Strict incr  Non-decr
Set.map              -        -        Strict incr  Non-decr
Map.fromList         No       Yes      Strict incr  Non-decr
Map.fromListWith     Yes      Yes      Never        Non-decr
Map.fromListWithKey  Yes      Yes      Never        Non-decr
Map.mapKeys          -        -        Strict incr  Non-decr
Map.mapKeysWith      -        -        Never        Non-decr
@meooow25
Copy link
Contributor Author

Rebased and updated benchmarks.

If there are no concerns, I would like to merge this soon, so that I can consider this fromList the baseline when trying out #1073.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@meooow25 @treeowl